Ensure `swap_nonoverlapping` is really always untyped #137412

scottmcm · 2025-02-22T05:05:03Z

This replaces #134954, which was arguably overcomplicated.

Fixes #134713

Actually using the type passed to ptr::swap_nonoverlapping for anything other than its size + align turns out to not work, so this goes back to always erasing the types down to just bytes.

(Except in const, which keeps doing the same thing as before to preserve @RalfJung's fix from #134689)

Fixes #134946

I'd previously moved the swapping to use auto-vectorization on bytes, but someone pointed out on Discord that the tail loop handling from that left a whole bunch of byte-by-byte swapping around. This goes back to manual tail handling to avoid that, then still triggers auto-vectorization on pointer-width values. (So you'll see <4 x i64> on x86-64-v3 for example.)

rustbot · 2025-02-22T05:05:12Z

r? @ibraheemdev

rustbot has assigned @ibraheemdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

library/core/src/ptr/mod.rs

rustbot · 2025-02-22T21:15:13Z

The Miri subtree was changed

cc @rust-lang/miri

src/tools/miri/tests/pass/stdlib/swap_nonoverlapping_is_untyped.rs

ibraheemdev · 2025-02-26T17:37:12Z

Passing this one along because I'm not the best person to review this. r? libs

jhpratt · 2025-02-26T23:19:57Z

I'm mostly sticking to the trivial PRs at the moment as my review capacity is limited. Re-rolling again.

r? libs

RalfJung

The code changes LGTM for the non-const path; see the comment for the const path.

I did not look at the tests; I think that needs an LLVM/codegen expert. @nikic maybe?

library/core/src/ptr/mod.rs

RalfJung · 2025-02-27T08:57:58Z

To be clear I only reviewed for correctness, I cannot judge whether all the tricks to get better performance actually work.

scottmcm · 2025-02-27T20:15:42Z

tests/assembly/x86_64-typed-swap.rs

+    // Ensure we do better than a long run of byte copies,
+    // see <https://github.com/rust-lang/rust/issues/134946>
+
+    // CHECK-NOT: movb
+    // CHECK-COUNT-8: movups{{.+}}xmm
+    // CHECK-NOT: movb
+    // CHECK-COUNT-4: movq
+    // CHECK-NOT: movb
+    // CHECK-COUNT-4: movl
+    // CHECK-NOT: movb
+    // CHECK: retq


note for reviewers: the codegen tests here are more about demonstrating what actually happens on a variety of types, and the exact details don't matter that much.

Reviewing the rust code is enough to know that LLVM will swap it, but for example here what we're trying to see is that it's not just a huge row of movbs like you can see in https://rust.godbolt.org/z/MKfxn1Tjr

tests/codegen/swap-large-types.rs

bors · 2025-03-01T15:54:03Z

☔ The latest upstream changes (presumably #137848) made this pull request unmergeable. Please resolve the merge conflicts.

bors · 2025-03-07T17:42:06Z

☔ The latest upstream changes (presumably #138155) made this pull request unmergeable. Please resolve the merge conflicts.

scottmcm · 2025-04-09T17:11:04Z

@bors try

Ensure `swap_nonoverlapping` is really always untyped This replaces rust-lang#134954, which was arguably overcomplicated. ## Fixes rust-lang#134713 Actually using the type passed to `ptr::swap_nonoverlapping` for anything other than its size + align turns out to not work, so this goes back to always erasing the types down to just bytes. (Except in `const`, which keeps doing the same thing as before to preserve `@RalfJung's` fix from rust-lang#134689) ## Fixes rust-lang#134946 I'd previously moved the swapping to use auto-vectorization *on bytes*, but someone pointed out on Discord that the tail loop handling from that left a whole bunch of byte-by-byte swapping around. This goes back to manual tail handling to avoid that, then still triggers auto-vectorization on pointer-width values. (So you'll see `<4 x i64>` on `x86-64-v3` for example.) --- try-jobs: x86_64-gnu-distcheck

bors · 2025-04-09T17:12:16Z

⌛ Trying commit b06a88f with merge bef076d...

In `swap_nonoverlapping_short` there's a new `debug_assert!`, and if that's enabled then the `alloca`s don't optimize out.

scottmcm · 2025-04-09T20:25:19Z

Only change is skipping the test when the new debug_assert! isn't disabled.
@bors r=cuviper rollup=iffy (yay codegen tests -- ought to work now, but...)

bors · 2025-04-09T20:25:22Z

📌 Commit 63dcac8 has been approved by cuviper

It is now in the queue for this repository.

Ensure `swap_nonoverlapping` is really always untyped This replaces rust-lang#134954, which was arguably overcomplicated. ## Fixes rust-lang#134713 Actually using the type passed to `ptr::swap_nonoverlapping` for anything other than its size + align turns out to not work, so this goes back to always erasing the types down to just bytes. (Except in `const`, which keeps doing the same thing as before to preserve `@RalfJung's` fix from rust-lang#134689) ## Fixes rust-lang#134946 I'd previously moved the swapping to use auto-vectorization *on bytes*, but someone pointed out on Discord that the tail loop handling from that left a whole bunch of byte-by-byte swapping around. This goes back to manual tail handling to avoid that, then still triggers auto-vectorization on pointer-width values. (So you'll see `<4 x i64>` on `x86-64-v3` for example.)

Rollup of 18 pull requests Successful merges: - rust-lang#137412 (Ensure `swap_nonoverlapping` is really always untyped) - rust-lang#138167 (Small code improvement in rustdoc hidden stripper) - rust-lang#138605 (Clean up librustdoc::html::render to be better encapsulated) - rust-lang#138682 (Allow drivers to supply a list of extra symbols to intern) - rust-lang#138904 (Test linking and running `no_std` binaries) - rust-lang#139423 (Suppress missing field error when autoderef bottoms out in infer) - rust-lang#139449 (match ergonomics: replace `peel_off_references` with a recursive call) - rust-lang#139507 (compiletest: Trim whitespace from environment variable names) - rust-lang#139530 (Remove some dead or leftover code related to rustc-intrinsic abi removal) - rust-lang#139560 (fix title of offset_of_enum feature) - rust-lang#139563 (emit a better error message for using the macro incorrectly) - rust-lang#139568 (Don't use empty trait names) - rust-lang#139580 (Temporarily leave the review rotation) - rust-lang#139589 (saethlin is back from vacation) - rust-lang#139592 (rustdoc: Enable Markdown extensions when looking for doctests) - rust-lang#139599 (Tracking issue template: fine-grained information on style update status) - rust-lang#139600 (Update `compiler-builtins` to 0.1.153) - rust-lang#139606 (Update compiletest to Edition 2024) r? `@ghost` `@rustbot` modify labels: rollup

Ensure `swap_nonoverlapping` is really always untyped This replaces rust-lang#134954, which was arguably overcomplicated. ## Fixes rust-lang#134713 Actually using the type passed to `ptr::swap_nonoverlapping` for anything other than its size + align turns out to not work, so this goes back to always erasing the types down to just bytes. (Except in `const`, which keeps doing the same thing as before to preserve ``@RalfJung's`` fix from rust-lang#134689) ## Fixes rust-lang#134946 I'd previously moved the swapping to use auto-vectorization *on bytes*, but someone pointed out on Discord that the tail loop handling from that left a whole bunch of byte-by-byte swapping around. This goes back to manual tail handling to avoid that, then still triggers auto-vectorization on pointer-width values. (So you'll see `<4 x i64>` on `x86-64-v3` for example.)

Rollup of 17 pull requests Successful merges: - rust-lang#137412 (Ensure `swap_nonoverlapping` is really always untyped) - rust-lang#138167 (Small code improvement in rustdoc hidden stripper) - rust-lang#138605 (Clean up librustdoc::html::render to be better encapsulated) - rust-lang#138682 (Allow drivers to supply a list of extra symbols to intern) - rust-lang#138904 (Test linking and running `no_std` binaries) - rust-lang#139423 (Suppress missing field error when autoderef bottoms out in infer) - rust-lang#139449 (match ergonomics: replace `peel_off_references` with a recursive call) - rust-lang#139507 (compiletest: Trim whitespace from environment variable names) - rust-lang#139530 (Remove some dead or leftover code related to rustc-intrinsic abi removal) - rust-lang#139560 (fix title of offset_of_enum feature) - rust-lang#139563 (emit a better error message for using the macro incorrectly) - rust-lang#139568 (Don't use empty trait names) - rust-lang#139580 (Temporarily leave the review rotation) - rust-lang#139589 (saethlin is back from vacation) - rust-lang#139592 (rustdoc: Enable Markdown extensions when looking for doctests) - rust-lang#139599 (Tracking issue template: fine-grained information on style update status) - rust-lang#139600 (Update `compiler-builtins` to 0.1.153) r? `@ghost` `@rustbot` modify labels: rollup

bors · 2025-04-10T20:19:14Z

⌛ Testing commit 63dcac8 with merge 0fe8f34...

bors · 2025-04-10T23:27:29Z

☀️ Test successful - checks-actions
Approved by: cuviper
Pushing 0fe8f34 to master...

github-actions · 2025-04-10T23:28:55Z

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 2205455 (parent) -> 0fe8f34 (this PR)

Test differences

Show 161 test diffs

Stage 0

ptr::test_ptr_swap_nonoverlapping_is_untyped: [missing] -> pass (J0)

Stage 1

[codegen] tests/codegen/swap-small-types.rs: pass -> ignore (ignored when std is built with debug assertions ((ptr::swap_nonoverlapping has one which blocks some optimizations))) (J2)
ptr::test_ptr_swap_nonoverlapping_is_untyped: [missing] -> pass (J3)

Stage 2

[codegen] tests/codegen/swap-small-types.rs: ignore (only executed when the architecture is x86_64) -> ignore (ignored when std is built with debug assertions ((ptr::swap_nonoverlapping has one which blocks some optimizations))) (J1)
[codegen] tests/codegen/swap-small-types.rs: pass -> ignore (ignored when std is built with debug assertions ((ptr::swap_nonoverlapping has one which blocks some optimizations))) (J4)

Additionally, 156 doctest diffs were found. These are ignored, as they are noisy.

Job group index

J0: mingw-check
J1: aarch64-gnu, arm-android, armhf-gnu, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1
J2: x86_64-gnu-llvm-19-3
J3: aarch64-apple, aarch64-gnu, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, test-various, x86_64-apple-1, x86_64-gnu, x86_64-gnu-aux, x86_64-gnu-llvm-19-1, x86_64-gnu-llvm-19-2, x86_64-gnu-llvm-19-3, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1
J4: x86_64-gnu, x86_64-gnu-llvm-19-1, x86_64-gnu-llvm-19-2, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1

Job duration changes

x86_64-apple-2: 7698.0s -> 5540.7s (-28.0%)
dist-x86_64-apple: 8790.2s -> 10668.2s (21.4%)
dist-aarch64-apple: 4710.8s -> 5210.3s (10.6%)
dist-powerpc-linux: 5754.1s -> 5238.7s (-9.0%)
x86_64-apple-1: 9359.0s -> 8539.6s (-8.8%)
dist-aarch64-msvc: 8678.7s -> 8043.2s (-7.3%)
x86_64-gnu-llvm-19-3: 7215.9s -> 6788.7s (-5.9%)
dist-x86_64-illumos: 5769.6s -> 6101.2s (5.7%)
dist-x86_64-freebsd: 5029.0s -> 4776.9s (-5.0%)
dist-aarch64-linux: 5463.4s -> 5240.3s (-4.1%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

rust-timer · 2025-04-11T01:30:09Z

Finished benchmarking commit (0fe8f34): comparison URL.

Overall result: ❌✅ regressions and improvements - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

If the regression was expected or you think it can be justified,
please write a comment with sufficient written justification, and add
@rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
If you think that you know of a way to resolve the regression, try to create
a new PR with a fix for the regression.
If you do not understand the regression or you think that it is just noise,
you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.4%, 1.7%]	5
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-1.1%, -0.3%]	9
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.2%	[-1.1%, 1.7%]	14

Max RSS (memory usage)

Results (primary 1.2%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.7%	[2.6%, 5.9%]	3
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-2.5%	[-2.6%, -2.4%]	2
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.2%	[-2.6%, 5.9%]	5

Cycles

Results (primary 0.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.7%	[0.7%, 0.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.7%, 0.7%]	1

Binary size

Results (primary 0.0%, secondary -0.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.1%, 0.8%]	17
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.3%, -0.0%]	37
Improvements ✅ (secondary)	-0.1%	[-0.1%, -0.1%]	3
All ❌✅ (primary)	0.0%	[-0.3%, 0.8%]	54

Bootstrap: 783.243s -> 781.425s (-0.23%)
Artifact size: 366.21 MiB -> 365.92 MiB (-0.08%)

Mark-Simulacrum · 2025-04-14T12:16:08Z

Correctness fix, relatively small set of regressed scenarios, compared to number of improvements. Marking as triaged.

Ensure `swap_nonoverlapping` is really always untyped This replaces rust-lang#134954, which was arguably overcomplicated. ## Fixes rust-lang#134713 Actually using the type passed to `ptr::swap_nonoverlapping` for anything other than its size + align turns out to not work, so this goes back to always erasing the types down to just bytes. (Except in `const`, which keeps doing the same thing as before to preserve `@RalfJung's` fix from rust-lang#134689) ## Fixes rust-lang#134946 I'd previously moved the swapping to use auto-vectorization *on bytes*, but someone pointed out on Discord that the tail loop handling from that left a whole bunch of byte-by-byte swapping around. This goes back to manual tail handling to avoid that, then still triggers auto-vectorization on pointer-width values. (So you'll see `<4 x i64>` on `x86-64-v3` for example.)

rustbot assigned ibraheemdev Feb 22, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 22, 2025

scottmcm mentioned this pull request Feb 22, 2025

Redo the swap code for better tail & padding handling #134954

Closed

This comment has been minimized.

Sign in to view

scottmcm force-pushed the redo-swap branch from 60d61a8 to 6964320 Compare February 22, 2025 05:36

This comment has been minimized.

Sign in to view

RalfJung reviewed Feb 22, 2025

View reviewed changes

library/core/src/ptr/mod.rs Show resolved Hide resolved

scottmcm force-pushed the redo-swap branch from 6964320 to 7ffb24c Compare February 22, 2025 21:06

RalfJung reviewed Feb 23, 2025

View reviewed changes

src/tools/miri/tests/pass/stdlib/swap_nonoverlapping_is_untyped.rs Outdated Show resolved Hide resolved

scottmcm force-pushed the redo-swap branch from 52ea885 to 631e58c Compare February 23, 2025 12:14

rustbot assigned jhpratt and unassigned ibraheemdev Feb 26, 2025

rustbot assigned cuviper and unassigned jhpratt Feb 26, 2025

RalfJung approved these changes Feb 27, 2025

View reviewed changes

library/core/src/ptr/mod.rs Show resolved Hide resolved

library/core/src/ptr/mod.rs Outdated Show resolved Hide resolved

scottmcm commented Feb 27, 2025

View reviewed changes

tests/codegen/swap-large-types.rs Show resolved Hide resolved

scottmcm force-pushed the redo-swap branch 2 times, most recently from c1b9092 to 9d37b4b Compare March 1, 2025 05:57

scottmcm force-pushed the redo-swap branch from 9d37b4b to a6de557 Compare March 7, 2025 05:26

This comment has been minimized.

Sign in to view

scottmcm force-pushed the redo-swap branch from 9648340 to b06a88f Compare April 9, 2025 16:15

skip tests/codegen/swap-small-types when debug assertions are on

63dcac8

In `swap_nonoverlapping_short` there's a new `debug_assert!`, and if that's enabled then the `alloca`s don't optimize out.

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 9, 2025

Zalathar mentioned this pull request Apr 10, 2025

Rollup of 18 pull requests #139612

Closed

Zalathar mentioned this pull request Apr 10, 2025

Rollup of 17 pull requests #139613

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 10, 2025

bors merged commit 0fe8f34 into rust-lang:master Apr 10, 2025
7 checks passed

rustbot added this to the 1.88.0 milestone Apr 10, 2025

This was referenced Apr 10, 2025

Try enabling precondition checks on ptr::{read,write} #129498

Draft

Add some track_caller info to precondition panics #129658

Draft

Implement Default for raw pointers #139535

Merged

scottmcm deleted the redo-swap branch April 11, 2025 00:47

rustbot added the perf-regression Performance regression. label Apr 11, 2025

Mark-Simulacrum added the perf-regression-triaged The performance regression has been triaged. label Apr 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure `swap_nonoverlapping` is really always untyped #137412

Ensure `swap_nonoverlapping` is really always untyped #137412

scottmcm commented Feb 22, 2025 •

edited

Loading

rustbot commented Feb 22, 2025

This comment has been minimized.

This comment has been minimized.

rustbot commented Feb 22, 2025

ibraheemdev commented Feb 26, 2025

jhpratt commented Feb 26, 2025

RalfJung left a comment

RalfJung commented Feb 27, 2025 via email

scottmcm Feb 27, 2025

bors commented Mar 1, 2025

This comment has been minimized.

bors commented Mar 7, 2025

scottmcm commented Apr 9, 2025

bors commented Apr 9, 2025

scottmcm commented Apr 9, 2025

bors commented Apr 9, 2025

bors commented Apr 10, 2025

bors commented Apr 10, 2025

github-actions bot commented Apr 10, 2025

Stage 0

Stage 1

Stage 2

rust-timer commented Apr 11, 2025

Mark-Simulacrum commented Apr 14, 2025

Ensure swap_nonoverlapping is really always untyped #137412

Ensure swap_nonoverlapping is really always untyped #137412

Conversation

scottmcm commented Feb 22, 2025 • edited Loading

Fixes #134713

Fixes #134946

rustbot commented Feb 22, 2025

This comment has been minimized.

This comment has been minimized.

rustbot commented Feb 22, 2025

ibraheemdev commented Feb 26, 2025

jhpratt commented Feb 26, 2025

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Feb 27, 2025 via email

scottmcm Feb 27, 2025

Choose a reason for hiding this comment

bors commented Mar 1, 2025

This comment has been minimized.

bors commented Mar 7, 2025

scottmcm commented Apr 9, 2025

bors commented Apr 9, 2025

scottmcm commented Apr 9, 2025

bors commented Apr 9, 2025

bors commented Apr 10, 2025

bors commented Apr 10, 2025

github-actions bot commented Apr 10, 2025

Test differences

Stage 0

Stage 1

Stage 2

Job duration changes

rust-timer commented Apr 11, 2025

Overall result: ❌✅ regressions and improvements - please read the text below

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Mark-Simulacrum commented Apr 14, 2025

Ensure `swap_nonoverlapping` is really always untyped #137412

Ensure `swap_nonoverlapping` is really always untyped #137412

scottmcm commented Feb 22, 2025 •

edited

Loading